Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor: Breadcrumbs Service #11590

Merged
merged 27 commits into from
Jan 5, 2022
Merged

Refactor: Breadcrumbs Service #11590

merged 27 commits into from
Jan 5, 2022

Conversation

ChaiWithJai
Copy link
Contributor

Background

Nomad and Waypoint both share a similar implementation of the Breadcrumbs service. Breadcrumbs are a navigation UI pattern to help users easily understand where they are when they’re inside of a deeply nested route.

Currently, Nomad and Waypoint use a Breadcrumbs service that uses the Router service to access the currentRoute and currentUrl and creates a computed derived state of a breadcrumbs array that is consumed by an AppBreadcrumbs component.

This pattern requires that Nomad and Waypoint both store state on the router. This breaks the Router/Controller boundary and the current use of the Router service breaks the Data Down, Actions Up paradigm by directly injecting the route into a service and passing the information into a component.

A Different Solution

Instead of violating the boundary between controllers and routers, we can use Renderless Components that consume our Breadcrumbs Service.

Our Breadcrumbs Service will be responsible for storing our breadcrumbs along with 2 functions: registering and deregistering breadcrumbs.

All Routes have templates that are rendered while the route is active, we can use a renderless component that consumes the Breadcrumbs Service and take advantage of 2 lifecycle methods: did-insert and will-destroy.

Whenever, a Route is initialized the component will intialize and register its trail of breadcrumbs to the breadcrumbs service. If we navigate away from the route, the component will unmount and unregister from our breadcrumbs trail.

Prior Art

We already follow this pattern in HCP

@github-actions
Copy link

github-actions bot commented Nov 30, 2021

Ember Asset Size action

As of 3bdf661

Files that got Bigger 🚨:

File raw gzip
nomad-ui.js +18.7 kB +2.37 kB
vendor.js +1.57 kB +249 B
nomad-ui.css +204 B +61 B

Files that stayed the same size 🤷‍:

File raw gzip
vendor.css 0 B 0 B

Searchable,
WithNamespaceResetting
) {
/* eslint-disable indent */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a follow-up task for this. Not valuable to tackle now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@github-actions
Copy link

github-actions bot commented Dec 15, 2021

Ember Test Audit comparison

main 3bdf661 change
passes 1238 1235 -3
failures 0 1 +1
flaky 0 1 +1
duration 10m 37s 007ms 000ms -10m 37s 007ms

@github-actions
Copy link

Ember Test Audit flaky tests

Ember Test Audit detected these flaky tests on 8a29d25:

  • Acceptance | task group detail: /jobs/:id/:task-group should list high-level metrics for the allocation
  • Acceptance | task group detail: /jobs/:id/:task-group second breadcrumb should link to the job for the task group

@ChaiWithJai ChaiWithJai requested a review from a team December 15, 2021 17:47
@github-actions
Copy link

Ember Test Audit flaky tests

Ember Test Audit detected these flaky tests on main:

  • Acceptance | task group detail: /jobs/:id/:task-group second breadcrumb should link to the job for the task group
  • Integration | Component | TopoViz: clicking on an allocation in a deeply nested TopoViz::Node will associate sibling allocations with curves
  • Integration | Component | TopoViz: when the count of sibling allocations is high enough relative to the node count, curves are not rendered
  • Integration | Utility | exec-socket-xterm-adapter: resizing the window passes a resize message through the socket

@jgwhite
Copy link

jgwhite commented Dec 16, 2021

@ChaiWithJai just to help us (@hashicorp/pl-applications-frontend) understand how to prioritize review for this:

  1. Is this blocking other work?
  2. Are there other motivations/priorities we should be aware of? (other works going on at the same time, that it might interact with)

@jgwhite
Copy link

jgwhite commented Dec 16, 2021

Copying from a Slack conversation with @ChaiWithJai, for reference:

Important commits:

  • Breadcrumb Service (Register/Deregister Pattern) 5f9fc61
  • Data Loading Component 5a1df04
  • Handle async relationships and remove need to resolve PromiseProxys ab460da

@ChaiWithJai
Copy link
Contributor Author

@ChaiWithJai just to help us (@hashicorp/pl-applications-frontend) understand how to prioritize review for this:

  1. Is this blocking other work?
  2. Are there other motivations/priorities we should be aware of? (other works going on at the same time, that it might interact with)

@jgwhite this is blocking 11436.

Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job on the refactoring!

I added some small comments and knit-picks, but the main ones are:

  • the is-active class got dropped from the active crumb
  • the a11y lint rule should still be set and used in tests
  • the auto-format in some of the template files made them a little weird and harder to read, I think it would be worth undoing those changes but I'm interesting in hearing what others think.

ui/app/components/breadcrumbs/job.hbs Outdated Show resolved Hide resolved
ui/app/controllers/csi/plugins/plugin.js Outdated Show resolved Hide resolved
Searchable,
WithNamespaceResetting
) {
/* eslint-disable indent */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ui/app/services/breadcrumbs.js Show resolved Hide resolved
ui/app/services/breadcrumbs.js Show resolved Hide resolved
Comment on lines -40 to +77
<th>ID</th>
<th>Created</th>
<th>Modified</th>
<th>Status</th>
<th>Client</th>
<th>Job</th>
<th>Version</th>
<th>CPU</th>
<th>Memory</th>
<th>
ID
</th>
<th>
Created
</th>
<th>
Modified
</th>
<th>
Status
</th>
<th>
Client
</th>
<th>
Job
</th>
<th>
Version
</th>
<th>
CPU
</th>
<th>
Memory
</th>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find that this (and other table header changes) make the file harder to read. Is this something that can be disabled in your code formater?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have major issues with my eslint and prettier config. I'm planning to tackle this in one swoop later. But, I can spend the day tomorrow and figure this out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, let me know if you need help. It may be easier to do it with an interactive rebase with git than manually revert them.

Comment on lines 8 to 15
get breadcrumb() {
return {
label: this.server.name,
args: ['servers.server', this.server.id],
};
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a criteria fro when to use a list vs. a single object? The other controllers seem to use a list even if there's only one item. Perhaps this is something that should be standardized

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no criteria of when to use a single object versus a list. We use a list when we're in a route that wants to show that it is part of a hierarchy that it's not actually a part of in router.js.

Generally, that's the only use case for using a list. Everything else should render to depict the route's model.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... I see the inconsistencies. Gonna re-do all the commits and clean this up tomorrow.

Comment on lines 32 to 34
rules: {
'ember-a11y-testing/a11y-audit-called': 'error',
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to keep this, and only disable in specific cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, great callout. Upon examining this package in our package.json this package is pointing to a commit in a repo and not the npm registry. No other HashiCorp product has this pattern. Its worth creating a follow-up issue to edit this behavior and be in line with HashiCorp's a11y standards.

Currently, this rule creates a lot of noise in our tests.

Comment on lines +61 to +62

await waitFor('[data-test-job-breadcrumb]');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move this up to keep all related assert close together.

);

await componentA11yAudit(this.element, assert);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably why the linter was complaining.

font-size: x-large;
}

li:last-child a.active {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LinkTo component is connected to the route layer and gets an active property if the route is active.

We use last-child if we're in a nested route because the parent route will also be set to active.

There's a namespace related issue where if the namespace query parameter is not forwarded then LinkTo will not be set to active. This is the same root cause as noted in 11327 and is noted by the Ember Core team.

We're watching the issue and all links will be properly highlighted when the issue is resolved by Ember Core and we don't think its worthwhile to create a workaround solution as this will be fixed by Core soon.

image

@vercel vercel bot temporarily deployed to Preview – nomad December 17, 2021 16:49 Inactive
@github-actions
Copy link

Ember Test Audit flaky tests

Ember Test Audit detected these flaky tests on 3bdf661:

  • Acceptance | task group detail: /jobs/:id/:task-group should list high-level metrics for the allocation

Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement! Just for reference, code formatting changes will be handled in #11754

@ChaiWithJai ChaiWithJai merged commit 38a3759 into main Jan 5, 2022
@ChaiWithJai ChaiWithJai deleted the e-ui/breadcrumbs-service branch January 5, 2022 22:46
@github-actions
Copy link

github-actions bot commented Nov 5, 2022

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants